List Posts by Rating #1

Ivopires · 2018-09-20T20:05:46Z

I chose the List Posts by Rating challenge from the list provided (list of programming challenges).

The challenge was developed using Python(2.7), which was the most familiar programming language, and the Django(1.11.15) framework.

Although, I had never used a Python framework for web application development, I thought it would be a good opportunity to use one.

In the README, you can find all the available application endpoints, as well as all the instructions necessary to run the application and its tests.

…ting into challenge

Mike-Sundays · 2018-09-21T13:49:10Z

Hi Ivo. Thanks for taking the time to do the challenge.
The team will now review your pull request :)

Mike-Sundays · 2018-09-21T14:03:57Z

README.md

@@ -1,2 +1,57 @@
-# ListPostsByRating
-App developed in Python, using the Django library
+[![Build Status](https://travis-ci.com/Ivopires/ListPostsByRating.svg?branch=challenge)](https://travis-ci.com/Ivopires/ListPostsByRating)


Kudos for the documentation!

Hey Miguel! Thanks for the feedback on the challenge

Mike-Sundays · 2018-09-21T14:07:37Z

list_posts/models.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+import numpy as np


Why did you feel the need to use numpy here?

I decided to used the numpy library in order to create more precise and consistent floats with 64-bit representation, which were used in the compute_score method implemented.

Mike-Sundays · 2018-09-21T14:09:14Z

list_posts/models.py

+
+    def compute_score(self):
+        """
+        The compute_score function will compute the Wilson-score Interval,


What are the advantages of using this score algorithm here, as opposed to a simpler one?

The main reasoning behind my decision was to solve the problem that was specified in the challenge description, where 2 posts with the same ratio (ex: 60 up and 40 down votes, and 600 up and 400 down) would have to be evaluated with a different score rather than a simple ratio between up/down votes (which could be a possible implementation of a simpler algorithm)

Mike-Sundays · 2018-09-21T14:14:37Z

list_posts/models.py

+    def __str__(self):
+        return self.post_text
+
+    def compute_score(self):


If the application started growing and adding a lot of new functionalities, would you keep the business logic here in the models?

In that case, I think the best solution is to implement a new class with the business logic implemented here inside the model, and then move this method to that class. And from there one, implement different classes with different purposes.

Mike-Sundays · 2018-09-21T14:16:00Z

list_posts/views.py

+
+def up_vote(request, post_id):
+    post = get_object_or_404(Post, pk=post_id)
+    post.up_votes += 1


What if we get two requests at the same time to update the same post with an upvote?

I completely neglected that fact when constructing the logic behind the up/down voting actions! On that case, if two concurrent requests would upvote a post there would happen an inconsistency on the up/down vote counting (where one of them would see the counter with the old value, for example 2 up votes, when the other had already incremented the counter to 3, for example)! But to resolve that problem, one could adopt a locking system to acquire the database lock before accessing and updating the database value of the post's upvotes.

Mike-Sundays · 2018-09-21T14:17:35Z

list_posts/views.py

+
+
+def up_vote(request, post_id):
+    post = get_object_or_404(Post, pk=post_id)


Is this the right place to hold the incrementing logic?

I think one of the possible solutions would be to implement the incrementing logic on an external class, although on the Django documentation the logic that lays behind the data that is presented to the user is implemented on the "view" class.

In order to gain some insight into how Django makes the split of the different layers, I checked some of the Django's documentation, including the FAQ, and some threads on the Stackoverflow (as an example stackoverflow post)

paulosilva86

Nice job Ivo. Let us ask you some questions in a code review style. Get back to us whenever you can. Take it easy with the beers 🍻 in Amsterdam 😉

paulosilva86 · 2018-09-21T14:10:18Z

challenge/list_posts/views.py


 def index(request):
-    return HttpResponse("Your posts will be posted here")
+    latest_posts_list = Post.objects.all()[:10]


This seems to fetch all Posts and then filter the first 10. Am I right?
If that's true, imagine the Posts table has millions of records. What could be the issue here? How would you solve it?

Hi Paulo, thanks for the review!

That could lead to a performance issue, but Django uses the Python's array-slicing syntax to limit the QuerySet to a certain number of results (in the code above, 10 rows), which is the equivalent to the to SQL's LIMIT and OFFSET.

At the beginning, I was also doubting about that syntax, but then after further reading of Django's documentation and other threads found that Django will use the limit clause in the SQL to fetch only the 10 rows.

paulosilva86 · 2018-09-21T14:12:33Z

challenge/list_posts/views.py

+            post.up_votes += 1
+        elif vote_type == 'downvote':
+            post.down_votes += 1
+        post.save()


Imagine you have 2 concurrent requests. Can you guarantee you won't lose votes with this solution? If not, how would you solve it?

I think this question is similar to the question made by @Mike-Sundays, which answer could be found above.

But in short, I think a race condition between two concurrent requests would arise and one plausible solution would be to use a locking mechanism that could acquire the lock to the database before accessing and updating the database values for the post's votes.

pedroswat · 2018-09-21T17:12:05Z

list_posts/urls.py

+urlpatterns = [
+    url(r'^$', views.index, name='index'),
+    url(r'^(?P<pk>[0-9]+)/$', views.PostDetailView.as_view(), name='post'),
+    url(r'^vote/(?P<post_id>[0-9]+)/$', views.vote, name='vote'),


Hi Ivo!
Thanks for your time!

Why do you implemented 2 ways to upvote/downvote? (vote/, upvote/, downvote/ )

Hi Pedro, thanks for the review!

Basically, the vote/ endpoint is responsible to handle the POST requests that are sent by the vote mechanism that was implemented in the UI, where the user could choose from up/down vote!
It was just created as an extra feature where the user naturally would navigate from the "index" endpoint to a specific post (hold by the /:post_id endpoint) and then had the possibility to up/down vote the post.

BernardoMG · 2018-09-24T17:18:35Z

list_posts/views.py

+
+    try:
+        vote_type = request.POST['vote']
+    except (KeyError, Post.DoesNotExist):


Hey dude! Thanks for your time!

Imagine that this application goes to production and eventually you'll have errors like this one. What would you do to be notified about exceptions/errors and it's details?

Hey Bernardo!

I think the more sensible way to keep up with the exceptions/erros and its details would be to integrate an error monitoring system, that could track and report the application's errors that impacted the user (like Rollbar and Sentry)

BernardoMG · 2018-09-24T17:22:42Z

README.md

@@ -1,2 +1,57 @@
-# ListPostsByRating
-App developed in Python, using the Django library
+[![Build Status](https://travis-ci.com/Ivopires/ListPostsByRating.svg?branch=challenge)](https://travis-ci.com/Ivopires/ListPostsByRating)


Ivopires added 16 commits September 17, 2018 17:43

Structure of the challenge completed! All endpoints working

9868c68

Added templates for the index.html and post.html! Post model created

92165d9

Starting models and views tests

38c8f48

Added final tests and README

a39af4e

Updated README

cabd661

Update README - fixing the link issue

a5d9bca

Update README

c62ce65

Update README.md

d58de16

Folder structure changed

66c7664

Update README.md

103b591

Update README.md

59c233c

Add travis config

b0fce1f

Merge branch 'challenge' of https://github.com/Ivopires/ListPostsByRa…

11f4d55

…ting into challenge

Fix travis config

a8348b1

Fix travis config

6644dcf

Update README.md

b2cbfb1

Mike-Sundays reviewed Sep 21, 2018

View reviewed changes

paulosilva86 reviewed Sep 21, 2018

View reviewed changes

pedroswat reviewed Sep 21, 2018

View reviewed changes

BernardoMG reviewed Sep 24, 2018

View reviewed changes



		def up_vote(request, post_id):
		post = get_object_or_404(Post, pk=post_id)

List Posts by Rating #1

Are you sure you want to change the base?

List Posts by Rating #1

Uh oh!

Conversation

Ivopires commented Sep 20, 2018

Uh oh!

Mike-Sundays commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulosilva86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants